Skip to content

Fix -Wstringop-truncation warnings#1730

Merged
aquynh merged 1 commit intocapstone-engine:v4from
kazarmy:fix-stringop-truncation
May 11, 2021
Merged

Fix -Wstringop-truncation warnings#1730
aquynh merged 1 commit intocapstone-engine:v4from
kazarmy:fix-stringop-truncation

Conversation

@kazarmy
Copy link
Contributor

@kazarmy kazarmy commented Mar 5, 2021

This pr fixes a couple of -Wstringop-truncation warnings on the v4 branch, found by compiling the code using the optimized asan build of rizinorg/rizin#260 (https://github.com/rizinorg/rizin/runs/2031121204, View raw logs, search for "-Wstringop-truncation" and see first 2 hits):

2021-03-04T12:50:27.0361388Z In file included from /usr/include/string.h:495,
2021-03-04T12:50:27.0362944Z                  from ../subprojects/capstone-bundled/cs.c:16:
2021-03-04T12:50:27.0364887Z In function ‘strncpy’,
2021-03-04T12:50:27.0366271Z     inlined from ‘fill_insn’ at ../subprojects/capstone-bundled/cs.c:579:11:
2021-03-04T12:50:27.0368213Z /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ output may be truncated copying 31 bytes from a string of length 31 [-Wstringop-truncation]
2021-03-04T12:50:27.0369934Z   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
2021-03-04T12:50:27.0370668Z       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

2021-03-04T12:51:08.4437002Z In file included from /usr/include/string.h:495,
2021-03-04T12:51:08.4438758Z                  from ../subprojects/capstone-bundled/arch/Sparc/SparcInstPrinter.c:29:
2021-03-04T12:51:08.4440765Z In function ‘strncpy’,
2021-03-04T12:51:08.4442293Z     inlined from ‘Sparc_printInst’ at ../subprojects/capstone-bundled/arch/Sparc/SparcInstPrinter.c:361:3:
2021-03-04T12:51:08.4444167Z /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 64 equals destination size [-Wstringop-truncation]
2021-03-04T12:51:08.4448603Z   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
2021-03-04T12:51:08.4449426Z       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@kazarmy kazarmy mentioned this pull request Mar 5, 2021
Closed
@pranith
Copy link
Contributor

pranith commented Mar 6, 2021

Can you please explain the change? Why is one change reducing the size of copy while the other is increasing it to the buffer size?

@kazarmy
Copy link
Contributor Author

kazarmy commented Mar 6, 2021

My hypotheses on this is:

  1. For the first case, the compiler knows the size of instr (it's 64, in the declaration a few lines up) but does not know the size of mnem (it's a char pointer). Therefore, when the developer wrote:
strncpy(instr, mnem, sizeof(instr));

the compiler warned that specified bound 64 equals destination size aka "emm do you know that strncpy does not automatically null-terminate the destination, and that you need to leave space for a null terminator??"

Thus, the addition of the - 1.

  1. For the second case, the compiler knows the size of insn->mnemonic (a member of struct cs_insn) and the size of tmp->insn.mnemonic (a member of struct customized_mnem) and the size is the same for both i.e. CS_MNEMONIC_SIZE. Therefore, when the developer wrote
(void)strncpy(insn->mnemonic, tmp->insn.mnemonic, sizeof(insn->mnemonic) - 1);

the compiler probably saw that the developer isn't copying all of tmp->insn.mnemonic, and then warned that the output may be truncated copying 31 bytes from a string of length 31 aka "why aren't you copying the null terminator??".

Thus, the removal of the - 1.

For the actual truth, you probably need to ask the gcc team but the above appears reasonable to me.

@kazarmy kazarmy mentioned this pull request Mar 8, 2021
Merged
4 tasks
@aquynh aquynh merged commit 8635f65 into capstone-engine:v4 May 11, 2021
@aquynh
Copy link
Collaborator

aquynh commented May 11, 2021

merged, thanks!

kazarmy added a commit to rizinorg/capstone that referenced this pull request May 13, 2021
@kazarmy
Fix -Wstringop-truncation warnings (capstone-engine#1730) (#2)
0f6ee66
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kazarmy @pranith @aquynh